Skip to content

Enhance shimmer progress with performance metrics#202

Closed
krataratha wants to merge 1 commit into
colbymchenry:mainfrom
krataratha:patch-1
Closed

Enhance shimmer progress with performance metrics#202
krataratha wants to merge 1 commit into
colbymchenry:mainfrom
krataratha:patch-1

Conversation

@krataratha

Copy link
Copy Markdown

Added optional fields for estimated time remaining and items per second in IndexProgress interface. Enhanced createShimmerProgress function to calculate and pass performance metrics.

Added optional fields for estimated time remaining and items per second in IndexProgress interface. Enhanced createShimmerProgress function to calculate and pass performance metrics.
Copilot AI review requested due to automatic review settings May 20, 2026 08:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR enhances shimmer indexing progress reporting by adding performance metrics (throughput and ETA) to the progress payload.

Changes:

  • Extend IndexProgress with optional estimatedTimeRemainingMs and itemsPerSecond.
  • Track phase start time and compute metrics on each progress update.
  • Post an enriched progress update message to the worker.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +71 to 78
async stop() {
await worker.terminate();
}
};
}
lastPhase = progress.phase;

let percent = -1;
Comment on lines +64 to +68
worker.postMessage({
type: 'progress-update',
phaseName,
...enrichedProgress
});

export function createShimmerProgress(): ShimmerProgress {
let lastPhase = '';
let phaseStartTime = Date.now();
lastPhase = progress.phase;

// New Feature: Calculate performance metrics
const elapsedMs = Date.now() - phaseStartTime;
@colbymchenry

Copy link
Copy Markdown
Owner

Thanks for the idea, @krataratha — surfacing ETA and items/sec during indexing is a nice touch. Unfortunately this PR can't be merged as-is:

  • It doesn't compile. The diff closes createShimmerProgress partway through onProgress (an early };} plus a second stop()), leaving the original body — lastPhase, the percent/count logic, and the real stop() — dangling at the top level. tsc fails with 6 syntax errors, and there end up being two stop() methods.
  • The metrics never render. It posts { type: 'progress-update', ... } to the worker, but shimmer-worker.ts only handles update / finish-phase / stop — so the computed numbers are dropped, and phaseName there is out of scope. Actually showing ETA/items-per-sec needs changes in the worker's render() (where the progress line is drawn), which this PR doesn't touch.

The salvageable part is just the two optional fields on IndexProgress. Since making this real means implementing the rendering (and validating it on Windows, where the worker writes raw bytes to fd 1 and relies on getGlyphs() ASCII fallbacks), it's better done as a fresh, focused change than patched onto this one.

Closing as not-mergeable — but the feature idea is welcome if you'd like to take another run at it. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants